Switch to calling Payment.create api when processing a refund from AdditionalPayment form #14317
Conversation
|
(Standard links)
|
…ditionalPayment form This is in conjunction with a series of changes consolidating & adding testing for creating payments with the goal being that the logic is all in the Payment.create api/BAO & none of the logic remains in the CRM_Contribute_Form_AdditionalPayment form This change completes switching the refund action over. There is logic in there to update the Participant status to Registered when a refund is processed - seemingly regardless of what it's original status is or the appropriate status afterwards. I tested through the UI and if you create a participant record & then change the line items, thus reducing the cost and changing the contribution status to 'Pending Refund' the participant status remains 'Completed' so I think this was not working / not required already & I simply dropped it
| // Fetch the contribution & do proportional line item assignment | ||
| $params = ['id' => $this->_contributionId]; | ||
| $contribution = CRM_Contribute_BAO_Contribution::retrieve($params, $defaults, $params); | ||
| // @todo - this line needs to be moved to the Payment.create api - it's not form layer appropriate. | ||
| // testing required. |
| } | ||
| } | ||
| $trxnsData['participant_id'] = $participantId; | ||
| return civicrm_api3('Payment', 'create', $trxnsData)['id']; | ||
| } |
There was a problem hiding this comment.
Just to be on the safe side if $payment NOT IN 'refund', 'owed' should we throw a exception message on else{...} loop?
There was a problem hiding this comment.
GIven we don't throw one now I don't think we should add one in 'just in case'
| @@ -319,7 +320,7 @@ public static function filterUntestedTemplateVariables($params) { | |||
| * | |||
| * @return CRM_Financial_DAO_FinancialTrxn | |||
| */ | |||
| public static function recordRefundPayment($contributionId, $trxnData, $updateStatus) { | |||
| protected static function recordRefundPayment($contributionId, $trxnData, $updateStatus) { | |||
There was a problem hiding this comment.
Umm you missed a spot
CRM//Contribute/BAO/Contribution.php:4017: $financialTrxn = CRM_Financial_BAO_Payment::recordRefundPayment($contributionId, $trxnsData, $updateStatus);
which expect it to be public.
There was a problem hiding this comment.
@monishdeb I don't see that line.. at that line I see
if (!empty($financialTrxn)) {
Are you on latest?
There was a problem hiding this comment.
@eileenmcnaughton yes I am checking on the latest instance heres the line https://github.com/civicrm/civicrm-core/blob/master/CRM/Contribute/BAO/Contribution.php#L4017
There was a problem hiding this comment.
@monishdeb but isn't that line removed in this pr?
There was a problem hiding this comment.
Ahh right ... (arghhh).. sorry my bad :(
|
@monishdeb yay. Now we just need to sort out the positive payments... |
|
Yup :) |
Overview
Code consolidation around processing negative payments.
Before
Logic is on form layer for processing refunds through additional payment form
After
Logic moved to BAO layer
Technical Details
This is in conjunction with a series of changes consolidating & adding testing for creating payments with the goal
being that the logic is all in the Payment.create api/BAO & none of the logic remains in the CRM_Contribute_Form_AdditionalPayment form
This change completes switching the refund action over. There is logic in there to update the Participant status to
Registered when a refund is processed - seemingly regardless of what it's original status is or the appropriate status
afterwards.
I tested through the UI and if you create a participant record & then change the line items, thus reducing the cost
and changing the contribution status to 'Pending Refund' the participant status remains 'Completed' so I think
this was not working / not required already & I simply dropped it
Comments
@monishdeb @pradpnayak getting there....